tls: Introduce OpenSSL #2569

michael-redpanda · 2024-12-09T13:29:42Z

Introduces OpenSSL as an alternative TLS implementation to GnuTLS. This is a build-time configuration controlled by the CMake variable Seastar_USE_OPENSSL. The configure.py script has been updated to now have a --crypto-provider option. Valid arguments to that are OpenSSL and GnuTLS.

This implementation was released in Redpanda v24.2 on July 31st, and has been running on production clusters since.

Redpanda implemented these changes in order to provide a FIPS-compliant build to customers that require it (such as those wishing to undergo FedRAMP evaluation). OpenSSL was selected as it allows implementors to maintain the validation of the cryptographic module even when it's built from source.

OpenSSL's security policy
- See appendix A for instructions on how to build OpenSSL for FIPS
FIPS 140-2 Implementation Guidance
- See section G.5 Maintaining validation compliance of software or firmware cryptographic
  modules

No changes have been introduced to enable the FIPS provider for Seastar. It is up to the implementor to enable and use the FIPS cryptographic module if desired.

Fixes: #698

avikivity · 2024-12-09T14:14:36Z

@elcallio please review

Are there any functional differences?

Is hot reload of certificates supported?

Should we support gnutls and openssl in parallel?

michael-redpanda · 2024-12-09T14:39:58Z

Are there any functional differences?

The only major difference between the OpenSSL vs GnuTLS implementation is how the implementation is configured. With OpenSSL, there are 5 new methods that can be used to control it (compared to the single set_priority_string with GnuTLS):

set_cipher_string - used for controlling which ciphers are available in TLS1.2 and below (OpenSSL SSL_CTX_set_cihper_list)
set_ciphersuites - used for controlling which cipher suites are available in TLS1.3 (same doc page as above)
enable_server_precedence
set_minimum_tls_version - defaults to 1.0
set_maximum_tls_version - defaults to 1.3

There may be some other subtler differences, such as OpenSSL may be stricter about certificate contents (e.g. see 1744b66 - this required the CA cert to have CA:True)

Is hot reload of certificates supported?

Yes

elcallio

This is quite a big change. I've mostly added nits and some form comments here.
My biggest gripe with it is that library/session config is somewhat different between OSSL/gnutls. The latter uses a single string for config, OSSL is more complicated.
A caller needs to know which impl is used, and adjust his code accordingly.
It makes for more potential bugs when a dev somewhere changes things in one impl, but misses to do it in the other. I.e. we need to make sure CI tests both properly.

elcallio · 2024-12-09T14:33:35Z

src/seastar.cc

@@ -335,6 +335,7 @@ module : private;
 #include <seastar/net/virtio.hh>

 #include "net/native-stack-impl.hh"
+#include "net/tls-impl.hh"


Why is this included here?

This is necessary for when compiling with modules enabled

elcallio · 2024-12-09T14:34:26Z

src/net/tls-impl.hh

+public:
+    static std::unique_ptr<connected_socket_impl> get(connected_socket s) {
+        return std::move(s._csi);
+    }


Seems to me this should be just declaration, with impl in impl.cc

This was just moved from tls.cc to this file.

Yes. But that was a cc. This is a header.

elcallio · 2024-12-09T14:34:48Z

src/net/tls-impl.hh

+        if (_session && _session.use_count() == 1) {
+            _session->close();
+        }
+    }


Same goes for things like this...

Same as above. Code should not be in headers if it can be avoided (even internal, not very included ones)

elcallio · 2024-12-09T14:40:03Z

include/seastar/net/tls.hh

@@ -202,13 +212,53 @@ namespace tls {

        // TODO add methods for certificate verification

+#ifndef SEASTAR_USE_OPENSSL


Bit of a negative check. Should this not be SEASTAR_USE_GNUTLS?

That's actually not set as a compile definitions, but I think it should be. Needs to be added to target_compile_definitions in CMakeLists.txt. I'll make that update.

elcallio · 2024-12-09T14:51:20Z

src/net/ossl.cc

+
+        // This call is required to lower SSL's security level to permit TLSv1.0 and TLSv1.1
+        // See https://www.openssl.org/docs/man3.0/man3/SSL_CTX_set_security_level.html
+        SSL_CTX_set_security_level(ssl_ctx.get(), 0);


Should this be unconditional? In the gnutls impl, allowing or not allowing TLS versions is controlled by the prio string. You check min/max version above, does the level always need lowering?

I'll experiment with this and get back to you

Did a little bit of research on this function and what the level controls changes depending on the version of OpenSSL.

For OpenSSL 3.0, setting security level to 3 disables TLS1.0 and below, and setting it to 4 disables TLS1.1 and below (ref).

Starting with OpenSSL 3.1, level 1 disables TLS1.1 and below (ref).

elcallio · 2024-12-09T14:57:12Z

src/net/tls-impl.cc

+#endif
+
+#ifdef SEASTAR_USE_OPENSSL
+void tls::credentials_builder::set_cipher_string(const sstring& cipher_string) {


Why not place these in the openssl impl file?

That's a good idea, I'll move the definitions to the correct source file

elcallio · 2024-12-09T15:05:19Z

tests/unit/tls_test.cc

@@ -161,7 +163,29 @@ SEASTAR_TEST_CASE(test_x509_client_with_builder_system_trust_multiple) {
    });
 }

+static void set_priority_string(tls::credentials_builder & b, const sstring & prio, [[maybe_unused]] bool is_tls_v13 = false) {


Please break this into two (or three) distinct functions and adjust call sites instead. Calling this set_prio more amplifies the issue with library configuration not being equivalent between openssl and gnutls, and is more confusing (imho) at the call sites than just doing different things to accomplish roughly the same.

elcallio · 2024-12-09T15:08:46Z

src/net/tls-impl.hh

+/*
+ * Copyright 2015 Cloudius Systems
+ */
+#pragma once


Why is this file in public header dir? Seems to me it should only be required by internal compilation units, so should probably be under src.

It's not in the public header directory, it's in src/net

Doh, sorry, github confused me.

elcallio · 2024-12-09T15:12:14Z

src/net/ossl.cc

    // This function waits for the _output_pending future to resolve
    // If an error occurs, it is saved off into _error and returned
    future<> wait_for_output() {
+        _logger.trace("wait_for_output");


Why invent a new logger type when you could just add a formatter for the session object that pretty-prints the local/remote etc, and then just use tls_log::trace("{} wait_for_output")?

Thanks for the suggestion, great idea!

elcallio · 2024-12-09T15:13:17Z

tests/unit/tls_test.cc

+                // during the handshake before the client has fully
+                // closed its connection, then the get() call will
+                // succeed by return an empty buffer indicating EOF
+                BOOST_REQUIRE(res.size() == 0);


Seems like a good change, but not related to the rest of the PR

This test was being flaky when using OpenSSL. It would pass reliably in release but not in debug.

nit, might want to use BOOST_REQUIRE_EQUAL() for better postmortem debugging experience. as Boost.Test prints out the lhs and rhs of the comparison f the check fails.

Was the test enshrining incorrect behavior?

avikivity · 2024-12-09T15:32:46Z

The different configuration options mean one of two:

side-by-side support, at least for a transitional period
emulate priority strings for openssl (far from the focus of Seastar)

So I think we should choose one implementation, and deprecate the other after a transition period.

gnutls was chosen due to funky licensing and an unstable API from openssl, but I think that's behind us now. Given that, it's better to use the leader in the field rather than a follower.

mykaul · 2024-12-09T16:31:02Z

GnuTLS is not FIPS enabled, if compiled with it? (I see https://www.gnutls.org/manual/html_node/FIPS140_002d2-mode.html ) - what's missing?

michael-redpanda · 2024-12-09T20:18:35Z

GnuTLS is not FIPS enabled, if compiled with it? (I see https://www.gnutls.org/manual/html_node/FIPS140_002d2-mode.html ) - what's missing?

The FIPS flag for GnuTLS means that GnuTLS will work in a FIPS compliant way (e.g. rejecting any non FIPS approved crypto like DES or GOST), however that doesn't mean that its implementation was validated. GnuTLS doesn't provide a path to build it and maintain validation, OpenSSL does (see above links to OpenSSL's security policy).

elcallio · 2024-12-10T11:38:04Z

gnutls was chosen due to funky licensing and an unstable API from openssl, but I think that's behind us now. Given that, it's better to use the leader in the field rather than a follower.

While I can somewhat sympathize with the sentiment, this would effectively mean dropping the way we handle TLS config for clients/applications (i.e. usage of the prio string). Since at least one database application I know of exposes this in its customer config, you'd effectively be asking to require changing all customer TLS configs where such a prio is applied. Not sure how many they are and how complicated the configs are. You said a transition period, but not sure how to handle this, nor enforce a config migration with clients?

avikivity · 2024-12-10T12:12:33Z

gnutls was chosen due to funky licensing and an unstable API from openssl, but I think that's behind us now. Given that, it's better to use the leader in the field rather than a follower.

While I can somewhat sympathize with the sentiment, this would effectively mean dropping the way we handle TLS config for clients/applications (i.e. usage of the prio string). Since at least one database application I know of exposes this in its customer config, you'd effectively be asking to require changing all customer TLS configs where such a prio is applied. Not sure how many they are and how complicated the configs are. You said a transition period, but not sure how to handle this, nor enforce a config migration with clients?

I would not want to force a config migration.

Claude says this:

The gnutls-cli tool includes a "--priority-debug" option that can show detailed information about a priority string's settings, but there isn't a direct tool to convert between GnuTLS priority strings and OpenSSL configuration formats.

You can analyze the GnuTLS priority string components with gnutls-cli --priority-debug "PRIORITY STRING" and then manually map those to equivalent OpenSSL configurations based on the cipher suites and protocols shown.

For more control, you could write a script that parses the gnutls-cli output and maps the components to OpenSSL's configuration syntax, though you'd need to account for the differences in how each library names and groups their ciphers and protocols.

elcallio · 2024-12-10T12:39:13Z

Yes, but I am honestly very nervous about writing/maintaining a prio string translator. The mapping is not just cipher to cipher etc, it is a state machine in itself, disabling and adding ciphers, exchange modes etc.
As for using cli tool and manually map - that is what a customer would have to do if we changed the config approach.

avikivity · 2024-12-10T12:44:00Z

Yes, but I am honestly very nervous about writing/maintaining a prio string translator. The mapping is not just cipher to cipher etc, it is a state machine in itself, disabling and adding ciphers, exchange modes etc. As for using cli tool and manually map - that is what a customer would have to do if we changed the config approach.

I agree with that.

@tzach do you have any insight about priority string configuration across our fleet? Do we ever diverge from the default?

mykaul · 2024-12-10T12:48:04Z

Yes, but I am honestly very nervous about writing/maintaining a prio string translator. The mapping is not just cipher to cipher etc, it is a state machine in itself, disabling and adding ciphers, exchange modes etc. As for using cli tool and manually map - that is what a customer would have to do if we changed the config approach.

I agree with that.

@tzach do you have any insight about priority string configuration across our fleet? Do we ever diverge from the default?

Since we did not disable TLSv1.1 by default (not sure why), there's a good chance users do it - https://enterprise.docs.scylladb.com/stable/operating-scylla/security/client-node-encryption.html#priority-string-and-tlsv1-2-1-3-support

michael-redpanda · 2024-12-12T01:06:54Z

Force push fc616ee:

Added compile definition SEASTAR_USE_GNUTLS for when GnuTLS is the cryptographic provder
Updated #ifndef SEASTAR_USE_OPENSSL to instead be #ifdef SEASTAR_USE_GNUTLS
Move implementation specific credential builder methods to the implementation specific compilation units
Removed tls_session_logger and added a pretty-print function for the OpenSSL implementation of seastar::tls::session
Dynamically setting minimum security level passed to SSL_CTX_set_security_level

michael-redpanda · 2024-12-12T13:46:27Z

Force push 1a53361:

Added missing compile definition for SEASTAR_USE_GNUTLS when compiled to use modules
Added missing header include for fmt/ostream.hh when compiled to use modules

elcallio

Code looks ok, but again, I doubt I can really verify it just by looking. :-(
One huge downside with the openssl code is that imho it seems to make the interface code even cludgier - perhaps partially an aspect of the code it needs to emulate is from a gnutls universe, but I would still argue that a lot of the gnutls interfaces are a bit nicer.
Thus I would worry a little about maintenance here.

elcallio · 2024-12-16T13:27:02Z

src/net/tls-impl.hh

+public:
+    static std::unique_ptr<connected_socket_impl> get(connected_socket s) {
+        return std::move(s._csi);
+    }


Yes. But that was a cc. This is a header.

elcallio · 2024-12-16T13:27:35Z

src/net/tls-impl.hh

+        if (_session && _session.use_count() == 1) {
+            _session->close();
+        }
+    }


Same as above. Code should not be in headers if it can be avoided (even internal, not very included ones)

elcallio · 2024-12-16T13:28:07Z

include/seastar/net/tls.hh

        void set_session_resume_mode(session_resume_mode);
+        void set_priority_string(const sstring&);


Seems like a gratuitous change?

elcallio · 2024-12-16T13:28:18Z

include/seastar/net/tls.hh

@@ -235,6 +247,12 @@ namespace tls {
        template<typename Base>
        friend class reloadable_credentials;
        shared_ptr<impl> _impl;
+
+        // The following methods are provided so classes that inherity from


Nit: inherit

elcallio · 2024-12-16T13:35:25Z

src/net/ossl.cc

+            auto dn = extract_dn_information();
+            if (dn) {
+                std::string_view stat_str_view{stat_str};
+                if (stat_str_view.ends_with(" ")) {


Nit: I assume this is an openssl quirk of some sort? Maybe a while instead (instead of assuming just one space), or maybe even a proper back and front whitespace strip?

elcallio · 2024-12-16T13:39:18Z

src/net/ossl.cc

+        }
+
+        auto & min_tls_version = _creds->minimum_tls_version();
+        auto & max_tls_version = _creds->maximum_tls_version();


Nit: space between type and ref.

elcallio · 2024-12-16T13:41:09Z

src/net/ossl.cc

+            }
+        }
+
+        auto get_min_level = [&ssl_ctx]() {


Nit: Should be "max level" really, as we should consider us setting the max level we can use given the tls version bounds.

elcallio · 2024-12-16T13:44:40Z

src/net/ossl.cc

+BIO_METHOD* get_method() {
+    static thread_local bio_method_ptr method_ptr = [] {
+        auto ptr = tls::create_bio_method();
+        if (!ptr) {


Seems redundant - all paths in create throws if failed...

elcallio · 2024-12-16T13:47:01Z

src/websocket/server.cc

@@ -126,6 +130,30 @@ future<> connection::process() {

 static std::string sha1_base64(std::string_view source) {
    unsigned char hash[20];
+
+#ifdef SEASTAR_USE_OPENSSL


Could we instead make the function an external/internal-header-decl, and move the full impl into gnutls/openssl compilation units? This just adds to an already bad separation of concern/api exposure.

michael-redpanda · 2024-12-20T15:22:49Z

Force push 2f935ca:

Addressed issues from comments
Created GnuTLS & OpenSSL implementations for SHA1-Base64 used by websocket
Fixed nits

tchaikov · 2024-12-23T02:23:24Z

CMakeLists.txt

+  set(Seastar_USE_GNUTLS OFF)
+else()
+  set(Seastar_USE_GNUTLS ON)


could you please add space after set and else? just to be consistent with the rest of this CMake script.

src/net/tls-impl.cc

+ * under the License.
+ */
+/*
+ * Copyright 2015 Cloudius Systems


tchaikov · 2024-12-23T02:39:53Z

src/net/ossl.cc

@@ -79,6 +79,8 @@ module seastar;
 #include "net/tls-impl.hh"
 #endif

+template <> struct fmt::formatter<seastar::tls::session> : fmt::ostream_formatter {};


is it possible to implement the specialization of fmt::fornmatter<seastar::tls::session> without the operator<<? it would be better if we can avoid adding more operator<<, for two reasons:

avoid adding the unused operator<<

avoid the overhead of using a temporary ostream for formatting seastar::tls::session.

tchaikov · 2024-12-23T02:54:50Z

src/net/ossl.cc

+}
+
+template<>
+struct fmt::formatter<seastar::ossl_errc> : public fmt::formatter<std::string_view> {


instead of inheriting from fmt::formatter<std::string_view> , i'd suggest inheriting from fmt::formatter<string_view>. please see scylladb/scylladb@168ade7 for more details. we switched from this approach to the proposed one in the referenced commit in scylladb a while ago.

tchaikov · 2024-12-23T03:00:18Z

src/net/ossl.cc

@@ -770,6 +772,8 @@ class session : public enable_shared_from_this<session>, public session_impl {
    session(session_type t, shared_ptr<tls::certificate_credentials> creds,
            std::unique_ptr<net::connected_socket_impl> sock, tls_options options = {})
      : _sock(std::move(sock))
+      , _local_address(fmt::format("{}", _sock->local_address()))


nit, could use fmt::to_string(_sock->local_address()) instead. simpler this way. and slightly more performant.

tchaikov · 2024-12-23T03:00:26Z

src/net/ossl.cc

@@ -770,6 +772,8 @@ class session : public enable_shared_from_this<session>, public session_impl {
    session(session_type t, shared_ptr<tls::certificate_credentials> creds,
            std::unique_ptr<net::connected_socket_impl> sock, tls_options options = {})
      : _sock(std::move(sock))
+      , _local_address(fmt::format("{}", _sock->local_address()))
+      , _remote_address(fmt::format("{}", _sock->remote_address()))


tchaikov · 2024-12-23T03:01:32Z

src/net/ossl.cc

+                   session.get_type_string(),
+                   session.local_address(),
+                   session.remote_address());
+        return os;


as explained above, i'd prefer avoid adding more operator<< overloads unless they are actually necessary and used.

tchaikov · 2024-12-23T03:06:45Z

src/net/ossl.cc

            return make_ready_future();
        }
        return _in.get()
          .then([this](buf_type buf) {
              // Set EOF if it's empty
+              tls_log.debug("{} wait_for_input: buffer {}empty", *this, buf.empty() ? "is ": "");


i don't really understand this logging message:

if buf is empty, we print "wait_for_input: buffer is empty" -- looks good

if buf is not empty, we print "wait_for_input: buffer empty" -- what does this mean? shall we print "wait_for_input: buffer is not empty" or "wait_for_input: buffer not empty" instead?

Thanks, that was my mess up. Fixing it

tchaikov · 2024-12-23T03:25:43Z

.github/workflows/tests.yaml

+      mode: debug
+      enables: --enable-cxx-modules
+      enable-ccache: false
+      crypto_provider: OpenSSL


piggyback the tests with OpenSSL / GnuTLS might not be a great idea. because, we only enable the test step in

seastar/.github/workflows/test.yaml

Lines 110 to 112 in 613d8b3

- name: Test

if: ${{ ! contains(inputs.enables, 'cxx-modules') }}

run: ./test.py --mode=${{ inputs.mode }}

if "cxx-modules" is not enabled. in other words, these two jobs do not run any of the unit tests. so i'd suggest adding a dedicated job in tests.yaml for building with the OpenSSL backend, and keep the existing job of build_with_cxx_modules intact.

please allow me to provide more context here: because C++20 modules is currently an experimental feature, not all Seastar facilities are exposed as in the "seastar" C++20 module at this moment, and we only have a single "hello_cxx_module" test for testing the build with C++20 module. none of the unit tests is built with the "seastar" C++20 module at the time of writing.

if "cxx-modules" is not enabled. in other words, these two jobs do not run any of the unit tests.

understood

so i'd suggest adding a dedicated job in tests.yaml for building with the OpenSSL backend, and keep the existing job of build_with_cxx_modules intact.

I did this to ensure that the changes I made to implement OpenSSL (namely the addition of ossl.cc) would build when modules were enabled. Completely understand that the tests are not built.

michael-redpanda · 2025-01-15T17:38:24Z

Force push 47dfb11:

Fixed logic of get_security_level
- Returning anything >1 when using OpenSSL 3.0 will introduce key restrictions that may cause Seastar to be unable to connect with other endpoints
Addressed error in test_reload_certificates_with_only_shard0_notify

avikivity · 2025-01-22T19:17:50Z

Note test failures.

I'm conflicted here. On the one hand, the tls implementation has a user footprint in priority string or equivalent openssl config. On the other hand, that's a tiny difference and they're otherwise equivalent. OpenSSL would be my choice as the only supported library if there wasn't the user facing stuff.

Can we translate the priority string to openssl config? I asked Claude and it spewed a long python script, maybe that's good enough.

michael-redpanda · 2025-01-22T19:23:05Z

Note test failures

It appears all failures are from Seastar.unit.rpc, which I don't believe interacts with the TLS. Were they flaky before?

Can we translate the priority string to openssl config? I asked Claude and it spewed a long python script, maybe that's good enough.

It definitely could be done by setting all of these within an OpenSSL config file. You'd just have to be careful to tell OpenSSL where to find it so it doesn't attempt to locate and use the system default one (unless that's desired).

michael-redpanda · 2025-01-22T19:24:51Z

Can we translate the priority string to openssl config? I asked Claude and it spewed a long python script, maybe that's good enough.

I guess your question is "can I take the input toset_priority_string and turn that into the appropriate OpenSSL API calls"? It could be doable, but would take a while and I'm not sure if that's something anyone would really want to maintain.

avikivity · 2025-01-22T19:45:50Z

Can we translate the priority string to openssl config? I asked Claude and it spewed a long python script, maybe that's good enough.

I guess your question is "can I take the input toset_priority_string and turn that into the appropriate OpenSSL API calls"? It could be doable, but would take a while and I'm not sure if that's something anyone would really want to maintain.

Is there no equivalent textual configuration?

I wouldn't want to maintain it, but neither do I want to maintain two different tls implementations.

michael-redpanda · 2025-01-22T19:59:42Z

Is there no equivalent textual configuration?

AFAIK only through the OpenSSL config file which can be loaded either automatically by OpenSSL when it is initialized or can be controlled by the user by calling OSSL_LIB_CTX_load_config. And within there would be number of key-value pairs that would need to be set correctly.

Created tls-impl.cc and tls-impl.h which contains common structures and definitions that are not dependent on the underlying TLS mechanism. These changes set the stage for implementing other TLS providers. Signed-off-by: Michael Boquard <michael@redpanda.com>

This commit adds support for using OpenSSL, instead of GnuTLS, as the TLS provider within Seastar. To support this change, the configure script has been updated to allow users to select which cryptographic provider should be used by supply `--crypto-provider` and specificying either `OpenSSL` or `GnuTLS`. The OpenSSL implementation mirrors the GnuTLS implementation. Instead of using callbacks, a custom BIO was created to handle moving data on/off of the OpenSSL SSL session into the Seastar TLS session data sinks. When compiled for OpenSSL, the `certificate_credentials::set_priority_string` method is compiled out and replaced with the following: * `set_cipher_string` * `set_ciphersuites` * `enable_server_precedence` * `set_minimum_tls_version` * `set_maximum_tls_version` These methods are specific to OpenSSL. The github actions have been updated to run the full suite of tests against both cryptographic providers. `src/net/tcp.hh` and `src/websocket/server.cc` have been updated to use OpenSSL instead of GnuTLS, depending upon the build configuration. Signed-off-by: Michael Boquard <michael@redpanda.com>

Added pretty-print capabilities to seastar::tls::session for OpenSSL and added a number of log statements that may be helpful if debugging the implementation. Signed-off-by: Michael Boquard <michael@redpanda.com>

More recent versions of OpenSSL requrire CA certificates to have CA:true Signed-off-by: Michael Boquard <michael@redpanda.com>

Now handling situations where the get() call doesn't throw but does return an empty buffer indicating EOF. Signed-off-by: Michael Boquard <michael@redpanda.com>

TLS renegotiation was removed in TLSv1.3 due to issues around security. This change makes that the default behavior, however a method is exposed to re-enable it if desired. This was only added to OpenSSL as GnuTLS does not provide a means to fully disable renegotiation. Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda · 2025-06-24T19:43:48Z

Hi again @avikivity and @elcallio . Apologies for such a long delay in updating this PR. I've made some updates and rebased off of master.

michael-redpanda · 2025-07-21T12:27:44Z

Hi again @avikivity and @elcallio . Was wondering if there was anything I could do to assist in regards to this PR?

avikivity · 2025-07-21T12:31:14Z

I prefer replacing gnutls with openssl rather than adding yet another option. But perhaps we can't due to those minor configuration issues.

Maybe we can start a long deprecation cycle during which users will be encouraged to update their configuration.

avikivity · 2025-07-21T12:32:28Z

If we indeed start a deprecation cycle, it will have to be run-time selectable (so users can update their configuration, change the selector, restart). I don't know how difficult that is.

elcallio · 2025-07-21T12:36:54Z

From an API standpoint, I guess replacing the prio strings with some sort of abstracted algorithm-selection is the biggest hurdle (std::set<tls_algo> allow, forbid?). Once users migrate this, the rest should be fairly transparent/equivalent.

avikivity · 2025-07-21T12:37:12Z

src/net/tls-impl.hh

+    virtual future<session_data> get_session_resume_data() = 0;
+    virtual future<std::vector<certificate_data>> get_peer_certificate_chain() = 0;
+    virtual future<std::optional<sstring>> get_selected_alpn_protocol() = 0;
+};


Looks like this is easy to make run-time (start-time) selectable.

Lack of virtual destructor is justified by shared_ptr below, but please add a comment.

But: is shared_ptr called for? why not unique_ptr or lw_shared_ptr? Who's sharing?

Input and output.

avikivity · 2025-07-21T12:39:08Z

src/net/tls-impl.hh

+    session_ref& operator=(const session_ref&) = default;
+
+    shared_ptr<session_impl> _session;
+};


using session_ref = would be sufficient, no?

This is in current TLS impl. and no. The session ref has a special property that if it is the last one to release the session (i.e. it will cause destruct), it "resurrects" the session and creates a background BYE handshake cycle.

avikivity · 2025-07-21T12:39:45Z

src/net/tls-impl.hh

+};
+
+class tls_connected_socket_impl : public net::connected_socket_impl, public session_ref {
+public:


Why is-a relationship between tls_connected_socket_impl and session_ref?

Because it needs to create input/output sinks when asked.

avikivity · 2025-07-21T12:40:53Z

src/net/tls-impl.hh

+class tls_connected_socket_impl::source_impl: public data_source_impl, public session_ref {
+public:
+    using session_ref::session_ref;
+private:


I see where the sharing comes from. But lw_shared_ptr is sufficient. Or keep a reference to the connected_socket_impl.

socket_impl is stored with a unique ptr -> cannot be shared. Our IO design allows the input/output streams to exists independently of the creating socket. Thus the need to share the underlying session.

avikivity · 2025-07-21T12:45:06Z

include/seastar/net/tls.hh

+
+#ifdef SEASTAR_USE_OPENSSL
+        /**
+         * Used to set the cipher string for TLS versions 1.2 and below


If we are to co-support the two implementations, then we should always compile these, but throw something instead. Maybe std::bad_cast (we're "casting" gnutls to openssl or something).

avikivity · 2025-07-21T12:45:24Z

include/seastar/net/tls.hh

+         * for documentation on the format of the ciphersuites string.
+         */
+        void set_ciphersuites(const sstring&);
+


std::string_view is more flexible.

avikivity · 2025-07-21T12:46:11Z

include/seastar/net/tls.hh

@@ -629,6 +685,12 @@ namespace tls {
    extern const int ERROR_NO_CIPHER_SUITES;
    extern const int ERROR_DECRYPTION_FAILED;
    extern const int ERROR_MAC_VERIFY_FAILED;


Are those error numbers compatible? And specified somewhere?

avikivity · 2025-07-21T12:46:48Z

include/seastar/net/tls.hh

+        sstring _ciphersuites;
+        bool _enable_server_precedence = false;
+        std::optional<tls_version> _min_tls_version;
+        std::optional<tls_version> _max_tls_version;


Let's package those in openssl/gnutls sub-structs.

avikivity · 2025-07-21T12:47:36Z

src/net/ossl.cc

@@ -0,0 +1,2288 @@
+/*


Please use "openssl.cc" to make it immediately understood.

avikivity · 2025-07-21T12:48:20Z

src/net/ossl.cc

+// sufficiently large enough to avoid collision with OpenSSL BIO controls
+#define BIO_C_SET_POINTER 1000
+// Index into ex data for SSL structure to fetch a pointer to session
+#define SSL_EX_DATA_SESSION 0


Please use C++ constexpr variables instead.

michael-redpanda mentioned this pull request Dec 9, 2024

tls: Introduce OpenSSL michael-redpanda/seastar#1

Closed

elcallio reviewed Dec 9, 2024

View reviewed changes

michael-redpanda force-pushed the add-openssl-implementation branch from 5c709c2 to fc616ee Compare December 12, 2024 01:03

michael-redpanda requested a review from elcallio December 12, 2024 01:07

michael-redpanda force-pushed the add-openssl-implementation branch from fc616ee to 1a53361 Compare December 12, 2024 13:45

elcallio reviewed Dec 16, 2024

View reviewed changes

michael-redpanda force-pushed the add-openssl-implementation branch from 1a53361 to 2f935ca Compare December 20, 2024 15:20

michael-redpanda requested a review from elcallio December 20, 2024 15:20

tchaikov reviewed Dec 23, 2024

View reviewed changes

src/net/tls-impl.cc

* under the License.

*/

/*

* Copyright 2015 Cloudius Systems

This comment was marked as resolved.

Sign in to view

tchaikov reviewed Dec 23, 2024

View reviewed changes

michael-redpanda requested a review from tchaikov January 15, 2025 15:09

michael-redpanda force-pushed the add-openssl-implementation branch from cab469f to 47dfb11 Compare January 15, 2025 17:36

michael-redpanda added 6 commits June 24, 2025 13:47

tls: Create tls-impl

49a1489

Created tls-impl.cc and tls-impl.h which contains common structures and definitions that are not dependent on the underlying TLS mechanism. These changes set the stage for implementing other TLS providers. Signed-off-by: Michael Boquard <michael@redpanda.com>

tls: Added tls_logger and log statements

6e9e390

Added pretty-print capabilities to seastar::tls::session for OpenSSL and added a number of log statements that may be helpful if debugging the implementation. Signed-off-by: Michael Boquard <michael@redpanda.com>

test/tls: Using CA extension for generating CA

e62eebb

More recent versions of OpenSSL requrire CA certificates to have CA:true Signed-off-by: Michael Boquard <michael@redpanda.com>

tls/test: Updated test_reload_certificates

ee3a9c6

Now handling situations where the get() call doesn't throw but does return an empty buffer indicating EOF. Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda force-pushed the add-openssl-implementation branch from 47dfb11 to 63027a1 Compare June 24, 2025 19:42

avikivity reviewed Jul 21, 2025

View reviewed changes

src/net/ossl.cc

@@ -0,0 +1,2288 @@

/*

Copy link

Member

avikivity Jul 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use "openssl.cc" to make it immediately understood.

avikivity reviewed Jul 21, 2025

View reviewed changes

		@@ -202,13 +212,53 @@ namespace tls {

		// TODO add methods for certificate verification

		#ifndef SEASTAR_USE_OPENSSL

		void set_session_resume_mode(session_resume_mode);
		void set_priority_string(const sstring&);

	- name: Test
	if: ${{ ! contains(inputs.enables, 'cxx-modules') }}
	run: ./test.py --mode=${{ inputs.mode }}

tls: Introduce OpenSSL #2569

Are you sure you want to change the base?

tls: Introduce OpenSSL #2569

Uh oh!

Conversation

michael-redpanda commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avikivity commented Dec 9, 2024

Uh oh!

michael-redpanda commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elcallio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avikivity commented Dec 9, 2024

Uh oh!

mykaul commented Dec 9, 2024

Uh oh!

michael-redpanda commented Dec 9, 2024

Uh oh!

elcallio commented Dec 10, 2024

Uh oh!

avikivity commented Dec 10, 2024

Uh oh!

elcallio commented Dec 10, 2024

Uh oh!

avikivity commented Dec 10, 2024

Uh oh!

michael-redpanda commented Dec 9, 2024 •

edited

Loading

michael-redpanda commented Dec 9, 2024 •

edited

Loading